-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ml for synthesis prototype - Refactoring #260
Ml for synthesis prototype - Refactoring #260
Conversation
0a05375
to
a8843d9
Compare
a8843d9
to
3add8bd
Compare
3add8bd
to
de72eb9
Compare
9ebb5c2
to
0d5d1e6
Compare
0d5d1e6
to
3945599
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor stuff, yet important enough.
Suggested new TODOs:
- write all
make docker-dev-*
commands and update the ml/synthesis/README.md
src/NITTA/Synthesis/Method.hs
Outdated
This used to be (VarValTime v x t, UnitTag tag). See below for more info. | ||
-} | ||
type SynthesisMethodConstraints tag v x t = (VarValTimeJSON v x t, ToJSON tag, UnitTag tag) | ||
|
||
-- FIXME: Validate the type above, its usages and meaning in the context of changes described below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should've been either moved with the SynthesisMethodConstraints or removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't sure is I answer to this question or not, but the main purpose of explicit type signatures here is: https://ryukzak.github.io/nitta/haddock/nitta/NITTA-Synthesis-Method.html
Without type alias it transform in a mess.
Comment deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, got it. Yes, it's an answer, thank you.
It's a shame this type pinning caused me to spend another ~3-4 hours on debugging/leaning when ToJSON constraints had to be added. Compiler errors looked completely cryptic to a Haskell junior that was completely unaware those constraints exist :D
Probably, I would've stumbled upon them somewhere else anyway.
ml/synthesis/src/components/data_crawling/nitta/nitta_running.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Ilya Burakov <speedwatson@gmail.com>
Co-authored-by: Ilya Burakov <speedwatson@gmail.com>
Co-authored-by: Ilya Burakov <speedwatson@gmail.com>
Co-authored-by: Ilya Burakov <speedwatson@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there
ml/synthesis/src/consts.py
Outdated
@@ -49,3 +49,6 @@ def _find_root_dir(): | |||
MODELS_DIR = Path(_models_dir_env) if _models_dir_env else ML_SYNTHESIS_DIR / "models" | |||
|
|||
ML_BACKEND_BASE_URL_FILEPATH = ".ml_backend_base_url" | |||
|
|||
# high priority env var which overrides command provided in the config file | |||
NITTA_RUN_COMMAND_OVERRIDE = os.environ.get("NITTA_RUN_COMMAND", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used env var name (NITTA_RUN_COMMAND
) doesn't match
nitta/.github/workflows/ci.yml
Lines 246 to 247 in ecd35f9
env: | |
NITTA_RUN_COMMAND_OVERRIDE: nitta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also it may be better to do it like
class EnvVarNames:
MODELS_DIR = "NITTA_ML_SYNTHESIS_MODELS_DIR"
NITTA_RUN_COMMAND = "NITTA_RUN_COMMAND"
...
NITTA_RUN_COMMAND_OVERRIDE = os.environ.get(EnvVarNames.NITTA_RUN_COMMAND, None)
it's consistent with the
_models_dir_env = os.environ.get(EnvVarNames.MODELS_DIR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't fully understand the idea behind EnvVarNames
class, but consistency is an argument. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is quite boring, EnvVarNames.MODELS_DIR had to be reused in tests:
nitta/ml/synthesis/src/tests/test_smoke.py
Lines 80 to 86 in ffa9302
async with run_nitta_server( | |
EXAMPLES_DIR / "fibonacci.lua", | |
nitta_args=f'--score="ml_{model_name}" --method=NoSynthesis -e', | |
env={EnvVarNames.MODELS_DIR: tmp_models_dir.resolve()}, | |
) as nitta: | |
ml_scores = await _get_scores(await nitta.get_base_url()) | |
assert non_ml_scores != ml_scores |
So I preferred to do it this way instead of hardcoding :)
Class is just for namespacing.
Regardless, I think it's good to have a single place where all the env var names (that python modules understand and use) are listed together, so they're not scattered across the code as literals. If there ever will be a need to generate docs for them, it'll make it simpler.
Co-authored-by: Ilya Burakov <speedwatson@gmail.com>
To: https://github.com/ryukzak/nitta/pull/167/files
Changes overview:
NITTA_RUN_COMMAND
from env variable.TODOs:
score
field inNodeView
todefScore
(UI, Haskell, maybe ML backend)port
type fromInt
with magic-1
toMaybe Int
NITTA.Synthesis.Explore:subForestIO
).Makefile:ml-nitta
NITTA_ML_BACKEND_RUN_COMMAND
to provide more control. But I move it to the new issue because: